Skip to content
This repository was archived by the owner on Apr 12, 2021. It is now read-only.

feat: Add support for hardhat-deploy#249

Merged
smartcontracts merged 49 commits intomasterfrom
feat/hardhat-deploy-v2
Apr 8, 2021
Merged

feat: Add support for hardhat-deploy#249
smartcontracts merged 49 commits intomasterfrom
feat/hardhat-deploy-v2

Conversation

@smartcontracts
Copy link
Copy Markdown
Collaborator

@smartcontracts smartcontracts commented Feb 24, 2021

Description

Adding new support for hardhat-deploy as an alternative to our bespoke deployment system. hardhat-deploy is quite nice and generates very clean artifacts. Next step should be to remove the old deployment system when possible. Though we might have to do something about generating state dumps.

fixes #256
fixes #116

Contributing Agreement

@smartcontracts smartcontracts marked this pull request as draft February 25, 2021 01:19

const libAddressManager = await Proxy__OVM_L1CrossDomainMessenger.libAddressManager()
if (libAddressManager !== Lib_AddressManager.address) {
throw new Error(
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do some cool post-deploy verification checks now!

@smartcontracts smartcontracts marked this pull request as ready for review March 26, 2021 07:31
@smartcontracts smartcontracts changed the title wip: use hardhat-deploy feat: Add support for hardhat-deploy Mar 26, 2021
@smartcontracts smartcontracts added the Category: Enhancement New feature or request label Mar 26, 2021
@@ -0,0 +1,12 @@
export const predeploys = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nice to have imo would be to export these as contract objects that are already connected to the correct address

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, will leave that for a future PR.

ovmRelayerAddress: process.env.RELAYER_ADDRESS,
})
}

Copy link
Copy Markdown
Collaborator

@tynes tynes Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit as then needs to be deprecated

;(async () => {
   const addresses = await main()
   console.log(JSON.stringify(addresses, null, 2))
})().catch(err => {

})

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was following https://hardhat.org/guides/scripts.html. And the ;(...)() syntax was giving me a lot of headache with the linter so I went with this.

@smartcontracts smartcontracts marked this pull request as ready for review April 8, 2021 05:32
@smartcontracts smartcontracts force-pushed the feat/hardhat-deploy-v2 branch from 778251b to 0be6832 Compare April 8, 2021 16:13
Copy link
Copy Markdown
Collaborator

@ben-chain ben-chain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested a few nit changes, but looking great!

@@ -0,0 +1,3 @@
CONTRACTS_TARGET_NETWORK=
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we comment these for extra completion points? :D

process.exit(1);
});
const contracts = dirtree(
path.resolve(__dirname, `../deployments/custom`)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this "custom" really means "CI" right now

await registerAddress({
hre,
name: 'OVM_Proposer',
address: (hre as any).deployConfig.ovmSequencerAddress,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
address: (hre as any).deployConfig.ovmSequencerAddress,
address: (hre as any).deployConfig.ovmProposerAddress,

(even if these are the same now might as well(

}
)

await OVM_L1CrossDomainMessenger.initialize(Lib_AddressManager.address)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a clarifying comment here that this is technically not required? Just for auditability

signerOrProvider: deployer,
})

await OVM_L1ETHGateway.initialize(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto above -- comment on redundant initialization for auditability :)

ben-chain
ben-chain previously approved these changes Apr 8, 2021
Copy link
Copy Markdown
Collaborator

@ben-chain ben-chain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI!

Copy link
Copy Markdown
Collaborator

@ben-chain ben-chain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@smartcontracts smartcontracts merged commit 3c458de into master Apr 8, 2021
@smartcontracts smartcontracts deleted the feat/hardhat-deploy-v2 branch April 8, 2021 22:23
@smartcontracts smartcontracts restored the feat/hardhat-deploy-v2 branch April 9, 2021 03:30
@smartcontracts smartcontracts deleted the feat/hardhat-deploy-v2 branch April 9, 2021 03:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add progress bar to contract deployment Add nonce manager for quick deploys

6 participants